Skip to content

Refactor logic to remove db or list from config#1946

Merged
charisk merged 2 commits intomainfrom
charisk/refactor-remove-item-logic
Jan 11, 2023
Merged

Refactor logic to remove db or list from config#1946
charisk merged 2 commits intomainfrom
charisk/refactor-remove-item-logic

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Jan 10, 2023

Created helper functions in DbConfig and used those in DbConfigStore to break up the function and remove the use of compareSelectedKindIsEqual.

Checklist

N/A:

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@charisk charisk added the secexp label Jan 10, 2023
@charisk charisk requested a review from a team as a code owner January 10, 2023 11:29
norascheuch
norascheuch approved these changes Jan 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@norascheuch norascheuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for making these changes!

The tests look like a duplicate of what's in db-config-store though - can we only test the behaviour in one file? In the config store we can make sure to call the respective methods in db-config.

@charisk
Copy link
Copy Markdown
Contributor Author

charisk commented Jan 10, 2023

Looks good, thanks for making these changes!

The tests look like a duplicate of what's in db-config-store though - can we only test the behaviour in one file? In the config store we can make sure to call the respective methods in db-config.

I've followed the same pattern as for the rename functionality - in db-config.test we test pretty much all test cases, and in db-config-store.test we test only a couple. We could do some mocking/spying but it feels a bit of an overkill to me.

@charisk charisk enabled auto-merge (squash) January 10, 2023 14:54
@charisk charisk force-pushed the charisk/refactor-remove-item-logic branch from 203da85 to f570efc Compare January 10, 2023 15:35
@charisk charisk merged commit 0b6f2c2 into main Jan 11, 2023
@charisk charisk deleted the charisk/refactor-remove-item-logic branch January 11, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@norascheuch norascheuch norascheuch approved these changes

Assignees

No one assigned

Labels

secexp

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@charisk @norascheuch